-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Install ipfs as a normal extension #2328
Conversation
4f04181
to
2991d54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just had a couple minor comments.
@@ -83,6 +85,15 @@ void BraveDefaultExtensionsHandler::SetHangoutsEnabled( | |||
} | |||
} | |||
|
|||
bool BraveDefaultExtensionsHandler::IsExtensionInstalled( | |||
const std::string extension_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you probably want const std::string& here?
@@ -25,6 +25,8 @@ class BraveDefaultExtensionsHandler : public settings::SettingsPageUIHandler { | |||
void SetHangoutsEnabled(const base::ListValue* args); | |||
void SetIPFSCompanionEnabled(const base::ListValue* args); | |||
|
|||
bool IsExtensionInstalled(const std::string extension_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be a const method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
@yrliou Step 5: Toggle IPFS off again - do you mean off the IPFS from below popup? If so, after making IPFS off, IPFS companion extension is NOT disabled in brave://extensions. Can you please clarify the point 5 in the above test plan? Thanks! |
@GeetaSarvadnya No, it is not the option page but the switch we introduced in brave://settings. I've updated step 5 to be more clear, thanks. |
Fix brave/brave-browser#3950
Fix brave/brave-browser#4218
Instead of installing as a component extension, this PR installs ipfs companion using a webstore installer so manifest could be localized and the extension option page could be shown normally under brave://extensions.
Toggle on the switch would prompt to install IPFS companion extension if it was't installed before. If already installed, it will enable the extension.
Toggle off the switch would disable IPFS companion extension (but not uninstall it). To fully remove (uninstall) the IPFS companion extension after it is installed, remove the extension through brave://extensions.
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Click "Cancel" in the prompt, the IPFS companion switch should be changed back to off.
Toggle it on again, and click "Add extension" in the prompt to install it, IPFS companion extension should now be installed and enabled.
Right click IPFS companion switch icon to open options page, it should shown as
In brave://settings, toggle IPFS companion switch off again and visit brave://extensions, IPFS companion extension should be shown disabled.
Enable the IPFS companion switch toggle in brave://settings again, IPFS companion extension should be shown enabled in brave://extensions.
Remove IPFS companion extension in brave://extensions, IPFS companion extension switch toggle in brave://settings should be shown off.
Reviewer Checklist:
After-merge Checklist:
changes has landed on.